Skip to content

n_bins parameter to Evaluator() to control lift curve computation#56

Merged
JanBenisek merged 7 commits intodevelopfrom
plots
Mar 24, 2021
Merged

n_bins parameter to Evaluator() to control lift curve computation#56
JanBenisek merged 7 commits intodevelopfrom
plots

Conversation

@sborms
Copy link
Contributor

@sborms sborms commented Mar 23, 2021

fixes #35

  • added argument n_bins to the Evaluator() class, to allow flexibility in terms of choosing the number of lift curve bins to compute and hence plot
  • by default argument is at 10, so deciles, meaning nothing changes compared to before if the user ignores the argument

@sborms sborms requested review from JanBenisek and sandervh14 March 23, 2021 15:40
@sborms sborms changed the title added n_bins parameter to Evaluator() to allow control at which inter… n_bins parameter to Evaluator() to flexibly control lift curve computation Mar 23, 2021
@sborms sborms changed the title n_bins parameter to Evaluator() to flexibly control lift curve computation n_bins parameter to Evaluator() to control lift curve computation Mar 23, 2021
Copy link
Contributor

@JanBenisek JanBenisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but please could you add one unit test to see if the function _compute_lift_per_bin() really returns as many bins as asked? Might be straightforward, but let's try having a good test coverage, thanks!

@sborms
Copy link
Contributor Author

sborms commented Mar 24, 2021

Looks good, but please could you add one unit test to see if the function _compute_lift_per_bin() really returns as many bins as asked? Might be straightforward, but let's try having a good test coverage, thanks!

Series of small commits but the test is there.

Strange it gives conflicts - all new code is an addition, not a modification. You can resolve using the web editor and merge to develop.

@JanBenisek JanBenisek merged commit b60a013 into develop Mar 24, 2021
@JanBenisek JanBenisek deleted the plots branch March 24, 2021 15:55
@JanBenisek JanBenisek linked an issue Mar 24, 2021 that may be closed by this pull request
3 tasks
@JanBenisek JanBenisek added this to the v1.0.2 milestone Mar 24, 2021
@JanBenisek
Copy link
Contributor

@sborms thanks, all good, conflicts resolved (just some small annoyance).
Please do not forget to always add a project label and linked issue (right side).
I also deleted the dev branch from here (remote).

Copy link
Contributor

@sandervh14 sandervh14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I went over the code. No comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Number of deciles in evaluator plots

3 participants